feat(confighttp): add max_redirects configuration option #10877
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10877 +/- ##
=======================================
Coverage 91.45% 91.45%
=======================================
Files 438 438
Lines 23827 23837 +10
=======================================
+ Hits 21791 21801 +10
Misses 1658 1658
Partials 378 378 ☔ View full report in Codecov by Sentry. |
|
Changes look great, if you would please reduce the diff to avoid having git history be impacted too much. |
rogercoll
force-pushed
the
add_max_redirects_confighttp
branch
from
163192c to
26bd593
Compare
|
@atoulme The lint CI is failing after removing the auto formatting changes: Should we fix them here? Or in a new PR? |
My bad, it was because of a removed space in the diffs. Moving the PR ready for review |
|
@atoulme Any update on this PR? Any additional feedback would be really appreciated. Thanks for taking a look. |
config/confighttp/confighttp.go
Outdated
| func makeCheckRedirect(max *int) func(*http.Request, []*http.Request) error { | ||
| if max == nil { | ||
| return nil | ||
| } else if *max == 0 { |
There was a problem hiding this comment.
Is this case needed here? if max is set to 0, will the check at line 266 return on the first call the same as is this block here?
There was a problem hiding this comment.
Great catch! Actually, this branch is not needed and resulted in the same output when max_redirects was set to either 0 or 1. The max redirects function is always called when a redirect response is returned, meaning that at least one request has already been made. The variable via []*http.Request on line 265 will always be > 0.
If max_redirects is set to 0, the http client should not follow any redirect response (one request). If max_redirects is 1, only one redirect response should be followed (2 total requests). max_redirects = total_requests - 1
Fixed in c98fe0a
Thanks!
config/confighttp/confighttp.go
Outdated
| @@ -105,6 +105,9 @@ type ClientConfig struct { | |||
| HTTP2PingTimeout time.Duration `mapstructure:"http2_ping_timeout"` | |||
| // Cookies configures the cookie management of the HTTP client. | |||
| Cookies *CookiesConfig `mapstructure:"cookies"` | |||
|
|
|||
| // Maximum number of redirections to follow, if not defined, the Client uses its default policy, which is to stop after 10 consecutive requests. | |||
| MaxRedirects *int `mapstructure:"max_redirects"` | |||
There was a problem hiding this comment.
I think we can even keep it int with 10 by default. That should be cleaner. The only problem is it'll be a significant breaking change (no redirects) for a component that doesn't use NewDefaultClientConfig, but I don't think we have any of those in contrib. Maybe we can check... All components should use that anyway. WDYT?
There was a problem hiding this comment.
I am totally open to adding this, but I am concert about receivers initiating the config without using the default method (max_redirects will be set to 0). Also, the only difference is that maybe we should align with the Go library error message after 10 redirects:
func defaultCheckRedirect(req *Request, via []*Request) error {
if len(via) >= 10 {
return errors.New("stopped after 10 redirects")
}
return nil
}There is this ongoing effort to use the NewDefaultClientConfig: open-telemetry/opentelemetry-collector-contrib#35457
There was a problem hiding this comment.
Different message isn't a problem I thought. I would like it to be merged without a pointer after all exporters use the default constructor. This is the pattern we use in other configs I believe. Let me know if that's ok with you
There was a problem hiding this comment.
Sounds reasonable, let's keep it pointer free :) e6b62bb
Should we wait until open-telemetry/opentelemetry-collector-contrib#35457 is done?
There was a problem hiding this comment.
contrib tests failing due open-telemetry/opentelemetry-collector-contrib#36681
Co-authored-by: Dmitrii Anoshin <[email protected]>
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description These components are constructing `confighttp.ClientConfig` field by field and adding the corresponding defaults. Instead of using default values to construct a new struct, the new struct should already contain the defaults. This becomes an issue when adding new fields with defaults in the `ClientConfig` structure, see: open-telemetry/opentelemetry-collector#10877 Error in the core collector CI: ``` === FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s) config_test.go:200: Config mismatch (-expected +actual): &elasticsearchreceiver.Config{ ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"}, ClientConfig: confighttp.ClientConfig{ ... // 15 identical fields HTTP2PingTimeout: s"0s", Cookies: nil, - MaxRedirects: 0, + MaxRedirects: 10, }, MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}}, Nodes: {"_local"}, ... // 4 identical fields } ``` <!-- Issue number (e.g. #1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.-->
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description These components are constructing `confighttp.ClientConfig` field by field and adding the corresponding defaults. Instead of using default values to construct a new struct, the new struct should already contain the defaults. This becomes an issue when adding new fields with defaults in the `ClientConfig` structure, see: open-telemetry/opentelemetry-collector#10877 Error in the core collector CI: ``` === FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s) config_test.go:200: Config mismatch (-expected +actual): &elasticsearchreceiver.Config{ ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"}, ClientConfig: confighttp.ClientConfig{ ... // 15 identical fields HTTP2PingTimeout: s"0s", Cookies: nil, - MaxRedirects: 0, + MaxRedirects: 10, }, MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}}, Nodes: {"_local"}, ... // 4 identical fields } ``` <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.-->
Is there any reference document/label that tracks features we want to add after v1? |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description These components are constructing `confighttp.ClientConfig` field by field and adding the corresponding defaults. Instead of using default values to construct a new struct, the new struct should already contain the defaults. This becomes an issue when adding new fields with defaults in the `ClientConfig` structure, see: open-telemetry/opentelemetry-collector#10877 Error in the core collector CI: ``` === FAIL: . TestLoadConfig/elasticsearch (re-run 1) (0.00s) config_test.go:200: Config mismatch (-expected +actual): &elasticsearchreceiver.Config{ ControllerConfig: {CollectionInterval: s"2m0s", InitialDelay: s"1s"}, ClientConfig: confighttp.ClientConfig{ ... // 15 identical fields HTTP2PingTimeout: s"0s", Cookies: nil, - MaxRedirects: 0, + MaxRedirects: 10, }, MetricsBuilderConfig: {Metrics: {ElasticsearchBreakerMemoryEstimated: {Enabled: true}, ElasticsearchBreakerMemoryLimit: {Enabled: true}, ElasticsearchBreakerTripped: {Enabled: true}, ElasticsearchClusterDataNodes: {Enabled: true}, ...}, ResourceAttributes: {ElasticsearchClusterName: {Enabled: true}, ElasticsearchIndexName: {Enabled: true}, ElasticsearchNodeName: {Enabled: true}, ElasticsearchNodeVersion: {Enabled: true}}}, Nodes: {"_local"}, ... // 4 identical fields } ``` <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes <!--Describe what testing was performed and which tests were added.--> #### Testing <!--Describe the documentation added.--> #### Documentation <!--Please delete paragraphs that you did not use before submitting.-->
Description
Link to tracking issue
#10870
Testing
Unit testing
Documentation
README.md